refactor: tweak type annotations to make Pyright pass #2375
Conversation
falcon/_typing.py
Outdated
| @@ -104,6 +104,9 @@ async def __call__( | |||
| HeaderMapping = Mapping[str, str] | |||
| HeaderIter = Iterable[Tuple[str, str]] | |||
| HeaderArg = Union[HeaderMapping, HeaderIter] | |||
|
|
|||
| NarrowHeaderArg = Union[Dict[str,str], List[Tuple[str, str]]] | |||
There was a problem hiding this comment.
I don't understand why we need this. it's more specific that HeaderArg
There was a problem hiding this comment.
Ok, the reason is this code:
def _load_headers(headers: Optional[HeaderArg]) -> Headers:
"""Transform the headers to dict."""
if headers is None:
return {}
if isinstance(headers, dict):
return headers
return dict(headers)The header args were wide-typed as either Mapping[str, str] or Iterable[tuple[str, str]]. The isinstance(..., dict) is not enough to tell Mapping from Iterable[tuple]. I.e. for k,v in {('foo', 'bar') : 42, ('a', 'b'): 111} yields perfect ('foo', 'bar'), ('a', 'b') pairs matching the Iterable[tuple[str, str]]. That's the pyright complain.
The docs state the arguments to errors must be list or dict (not list-like or dict-like), I thought it would be the right thing to match the typing to runtime.
There was a problem hiding this comment.
this seems overly pedantic. it's likely fine to narrow HeaderArg to use Sequence[Tuple...] that should solve the complain
There was a problem hiding this comment.
I changed the the NarrowHeaderArg to Union[Mapping[], Sequence[]], pyright is ok with it.
Changing the very HeaderArg from Iterable to Sequence is wrong imho, since it's used in set_headers.
The docs state the set_headers accepts iterable of tuples but sequence will disallow using generators there.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2375 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 7725 7738 +13
Branches 1071 1071
=========================================
+ Hits 7725 7738 +13 ☔ View full report in Codecov by Sentry. |
|
I had also made some changes. Will create a new PR |
|
No complains from pyright on my check in both standart and basic modes. I think meth: Anno = getattr(obj, 'meth_name', None);
if meth:
meth()vs if hasattr(obj, 'meth_name'):
ob.meth_name()is still better from both the typecheck and performance standpoints: one attribute lookup vs two.
|
|
I agree getatrr. I will cherry pick your chane in the other one |
vytas7
changed the title
Summary of Changes
This is not intended to be merged as is, I'm not that familiar with the falcon codebase.
The code is passing the Pyright checks in basic mode.
In standard mode there are a few more errors. These are mostly related to the unbound variables defined only in some branch of if/else. Pyright is strict here and insist for variables to be always defined. I "fixed" one of these errors in CompiledRouter.
I run the tests. All of them are either green or skipped (?).
The type for headers of falcon.errors was made more strict:
dict[str,str] | list[tuple[str, str]instead ofMapping[str, str] | Iterable[tuple[str, str]]. The catch is that dict is iterable too, so dict keyed by tuple of two strings (i.e.dict[tuple[str, str], Any]) is accepted, while should not.I moved _logger to the separate module. The _logger was used in falcon.asgi/app, but root falcon module was never imported there.
I replaced
hasattr()applied to some optional methods withgetattr(). Pyright is strict here too:hasattrsuccess means nothing :-)